Skip to content

fix flacky wallet test#143

Merged
ChrisSon15 merged 4 commits intobisq-network:mainfrom
ChrisSon15:fix-flacky-wallet-tests
Apr 1, 2026
Merged

fix flacky wallet test#143
ChrisSon15 merged 4 commits intobisq-network:mainfrom
ChrisSon15:fix-flacky-wallet-tests

Conversation

@ChrisSon15
Copy link
Copy Markdown
Collaborator

@ChrisSon15 ChrisSon15 commented Mar 31, 2026

by tracking all broadcasted transactions and making sure when we mine a block, that we wait for the indexer that it has indexed all transactions before we return from mine_block.
Internally I add all transactions which are broadcasted by TestEnv to a Vec. When mine_block() is called, I wait for all these transaction to be seen in Electrs (and bitcoind), to assume they are fully synced again. then I remove the transactions from the Vec and return from mine_block()

…aking sure when we mine a block, that we wait for the indexer that it has indexed all transactions before we return from mine_block.
@ChrisSon15 ChrisSon15 requested a review from sdmg15 March 31, 2026 22:50
…o be fixed. I set it to ignired for now with a TODO
@stejbac
Copy link
Copy Markdown
Contributor

stejbac commented Apr 1, 2026

Are there no Clippy warnings after these changes? There are a number of places where let env = .. has been replaced with let mut env = .. but env does not appear to be used/referenced mutably, which ought to trigger rustc/Clippy warnings.

Comment thread rpc/tests/wallet_service.rs Outdated
// TODO fix this test, I guess we need to rewrite it, may be the whole streaming of transaction
// events.
#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
#[ignore = "needs to be fixedy"]
Copy link
Copy Markdown
Contributor

@stejbac stejbac Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a typo here.

Copy link
Copy Markdown
Contributor

@sdmg15 sdmg15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tAck

env.mine_block()?;

wallet.sync_all(data_source)?;
wallet.sync_all(env.bdk_electrum_client())?;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this separation needed?
Wouldn't this return the same client?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

borrow checker complains, if I store it is Data_source. I dont remember the exact details, if you want to see then just change it quickly back and see the error message. Basically a consequence that with &mut be can have only one reference at a time.

fn test_q_tik() -> anyhow::Result<()> {
let env = TestEnv::new()?;
let mut env = TestEnv::new()?;
// env.start_explorer_in_container()?;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove these comments if not needed anymore?

@sdmg15
Copy link
Copy Markdown
Contributor

sdmg15 commented Apr 1, 2026

Are there no Clippy warnings after these changes? There are a number of places where let env = .. has been replaced with let mut env = .. but env does not appear to be used/referenced mutably, which ought to trigger rustc/Clippy warnings.

It's not because the receiver of the function call have changed. see testenv/lib.rs

@stejbac
Copy link
Copy Markdown
Contributor

stejbac commented Apr 1, 2026

It's not because the receiver of the function call have changed. see testenv/lib.rs

Yes, sorry, I see now. By adding the extra mempool field to TestEnv, some of its methods now need to take it by mutable reference.

@ChrisSon15
Copy link
Copy Markdown
Collaborator Author

Are there no Clippy warnings after these changes? There are a number of places where let env = .. has been replaced with let mut env = .. but env does not appear to be used/referenced mutably, which ought to trigger rustc/Clippy warnings.

Internally I add all transactions which are broadcasted by TestEnv to a Vec. When mine_block() is called, I wait for all these transaction to be seen in Electrs (and bitcoind), to assume they are fully synced again. then I remove the transactions from the Vec and return from mine_block().

So in the new model even simple method calls on TestEnv will change the internal state. I tried around using RefCell<Vec<...>> internally, but I think its better to make clear even to the caller, that most operations now do change the internal state. So clippy doesnt warn, because I change most of the API to require &mut.

@ChrisSon15 ChrisSon15 merged commit 8ef044d into bisq-network:main Apr 1, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants